-
Notifications
You must be signed in to change notification settings - Fork 1k
Add Optimization Barriers #4763
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add Optimization Barriers #4763
Conversation
767981a to
33d5138
Compare
|
Is there a background discussion on this cell somewhere? I'm interested in it as it's similar to a |
|
I brought it up at the Dev JF on monday but we didn't write down too much about the conclusions. The gist of it is that there are some applications for Yosys where it is more important to retain some amount of the structural information rather than just the semantic. This has caused issues like #3426 in the past and has given me similar issues recently where I want to structurally rewrite the drivers for some nodes which causes semantic changes to the design for formal verification. The alternative approach to something like optimization barriers is to not do the optimizations in the first place, but in my experience there tends to be lots of logic involved with private wires that cause a blowup in design size and can be cleaned away straightforwardly, the structure with respect to public wires is usually all that matters. Am open to any suggestions/critiques about how to go about this, this is just a draft for one way it can be done. |
|
I think I may need a bit more care with how without optbarriers this generated a if (rst)
barrier_in <= barrier_out;
else
barrier_in <= a;
I think the right solution to this is that as well as rewriting signals to come from a barrier when they appear on the |
Since variants of this were brought up independently and for different use cases, I made an attempt now to summarize the motivating use cases that I'm aware of and how it would apply there:
|
|
Thanks for such a thorough analysis, @jix! |
|
I will also chime in and add that, if I'm understanding this pull correctly, this would be an incredibly useful feature for my project. I'm working on a triple modular redundancy plugin, and one major unresolved problem is passes like If I could instead annotate cells with |
f864b7a to
6532cf6
Compare
|
Have fixed the process issue but it adds a reasonable bit more complexity. The idea is that a path through a set of assignments in a process from a variable to itself should see the pre-barrier version of itself, as this is needed in I have an example on a real design that matches the pattern I commented earlier in the thread where this more careful rewriting is needed to prevent an On a reasonable sized design running The flop/latch inferences are the same (which is not true without the more complex process rewriting) and the rest of the numbers seem to be in the reasonable ranges for what id expect, although I haven't done a proper equivalence check or anything yet and this could probably do with tests. Not sure when I will get much time to add tests at the moment. Other than that I think the logic should be done so I will mark as ready for review for now. |
povik
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have reviewed all the bits outside optbarries.cc, I think we need to fix the bmt2 $buf handling
| register_reverse_wire_map(y_id, cell->getPort(ID::Y)); | ||
| continue; | ||
| } | ||
| if (cell->type == ID($pos)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's strange this interprets $pos without looking at the A_SIGNED parameter, but if there's a bug it's pre-existing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think its fine to leave as is for now
|
|
||
| if (cell->type == ID($not)) return export_bvop(cell, "(bvnot A)"); | ||
| if (cell->type == ID($pos)) return export_bvop(cell, "A"); | ||
| if (cell->type.in(ID($pos), ID($buf), ID($barrier))) return export_bvop(cell, "A"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will crash when given a $buf or $barrier as those don't have A_SIGNED
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, have updated export_bvop to only check for A_SIGNED if it exists
kernel/satgen.cc
Outdated
| } | ||
|
|
||
| if (cell->type.in(ID($pos), ID($buf), ID($neg))) | ||
| if (cell->type.in(ID($pos), ID($buf), ID($barrier), ID($neg))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think by supplying a SAT model for $barrier to the optimization passes you are running the risk of barrier leakage. I would make the model opt-in, not sure how to do it best though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about how to opt in - had added this so I could write equiv_opt tests but we have no easy way of separating "you can use this in sat models for verification" from "you can use this in sat models for optimization". I've removed this for now, as well as the similar implementation in cellaigs.cc because that meant barriers could be lost when running aigmap.
passes/techmap/flatten.cc
Outdated
| log(" 'hdlname' attribute.\n"); | ||
| log("\n"); | ||
| log(" -barriers\n"); | ||
| log(" Use $barrier cells to connect flattened modules to their surrounding\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"introduce optimization barriers when a flattened module drives a public wire" ?
passes/techmap/flatten.cc
Outdated
|
|
||
| for (int i = 0; i < GetSize(new_conn.first); i++) { | ||
| const auto lhs = new_conn.first[i], rhs = new_conn.second[i]; | ||
| auto& sigsig = !lhs.is_wire() || !lhs.wire->name.isPublic() ? skip_conn : barrier_conn; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think lhs.is_wire() shouldn't happen here, we could add an assert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Err, !lhs.is_wire()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh yes I had thought you could have this with something like tristates, but I double checked and such a scenario does get caught by the check a few lines earlier and thrown as an error, so it is fine to assume its non-const here. Even though its checked just before I'll add an assertion in case the two ever end up separated by refactoring.
3ed98d6 to
784751f
Compare
|
Am going to take a look at this again - life got busy and it slipped off my todo list to finish responding to reviews here. Have just rebased it onto main and will address the comments + add some tests now. |
784751f to
9dc6f5d
Compare
|
As to a discussion I had with @widlarizer about whether this meets the requirements for something like LLVM's
The corrolary of this is that a barrier implies freeze but is stronger (it prevents reasoning based on the input value). If we want the freeze behaviour this can be added later either with a separate cell or maybe an attribute/parameter of |
That is consistent with what I've been thinking of. When I was writing my comment above I was considering a The first bullet point of that talks about a variant that is transparent only if the input is statically known to be non- If you combine the |
9dc6f5d to
3bcca55
Compare
georgerennie
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have addressed the review comments and added some basic testcases.
I also did a best effort test that this didn't seem to break the changed backends with the following script that tries to output picorv32 with and without barriers - some of that is a bit verified by hand but it seems to give reasonable results.
read_verilog picorv32.v
prep -top picorv32 -barriers -flatten
pmuxtree
autoname
select -assert-any t:$barrier
design -push-copy
# Write out with barriers
write_verilog barriers.v
write_smt2 barriers.smt2
write_btor barriers.btor2
write_firrtl barriers.firrtl
optbarriers -remove
select -assert-none t:$barrier
# Write out without
write_verilog nobarriers.v
write_smt2 nobarriers.smt2
write_btor nobarriers.btor2
write_firrtl nobarriers.firrtl
exec -expect-return 0 -- bash -c "diff <(sort barriers.v) <(sort nobarriers.v)"
# The amount of concats/slices changes slightly but I don't think this is a big
# deal, its to do with how the backend exports ranges of signals I think
exec -expect-return 0 -- bash -c "diff <(awk '{print \$2}' barriers.btor2 | sort | rg -v '(concat|slice)') <(awk '{print \$2}' nobarriers.btor2 | sort | rg -v '(concat|slice)')"
# Firrtl export seems to be kinda broken in general so these aren't the same,
# but from looking at the diff the changes don't seem to be due to the barriers
# exec -expect-return 0 -- bash -c "diff barriers.firrtl nobarriers.firrtl"
# Synth down to aig and do the same
design -pop
# slightly random commands to create some kind of aig without flops,
# doesn't matter too much what happens here
expose -evert-dff
delete t:$dff
synth
delete t:$_DFFE_PP_
aigmap
abc -g aig
aigmap
stat
write_aiger2 barriers.aig
optbarriers -remove
select -assert-none t:$barrier
write_aiger2 nobarriers.aig
exec -- abc -c "cec -v nobarriers.aig barriers.aig"
passes/techmap/flatten.cc
Outdated
|
|
||
| for (int i = 0; i < GetSize(new_conn.first); i++) { | ||
| const auto lhs = new_conn.first[i], rhs = new_conn.second[i]; | ||
| auto& sigsig = !lhs.is_wire() || !lhs.wire->name.isPublic() ? skip_conn : barrier_conn; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh yes I had thought you could have this with something like tristates, but I double checked and such a scenario does get caught by the check a few lines earlier and thrown as an error, so it is fine to assume its non-const here. Even though its checked just before I'll add an assertion in case the two ever end up separated by refactoring.
| register_reverse_wire_map(y_id, cell->getPort(ID::Y)); | ||
| continue; | ||
| } | ||
| if (cell->type == ID($pos)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think its fine to leave as is for now
|
|
||
| if (cell->type == ID($not)) return export_bvop(cell, "(bvnot A)"); | ||
| if (cell->type == ID($pos)) return export_bvop(cell, "A"); | ||
| if (cell->type.in(ID($pos), ID($buf), ID($barrier))) return export_bvop(cell, "A"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, have updated export_bvop to only check for A_SIGNED if it exists
kernel/satgen.cc
Outdated
| } | ||
|
|
||
| if (cell->type.in(ID($pos), ID($buf), ID($neg))) | ||
| if (cell->type.in(ID($pos), ID($buf), ID($barrier), ID($neg))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about how to opt in - had added this so I could write equiv_opt tests but we have no easy way of separating "you can use this in sat models for verification" from "you can use this in sat models for optimization". I've removed this for now, as well as the similar implementation in cellaigs.cc because that meant barriers could be lost when running aigmap.
* This acts as an optimization barrier, limiting the rewriting allowed * For now this forbids all optimization, but attributes can be added to opt in to specific optimizations as required
* This uses $barrier optimization barriers to connect wires into the flattened module instead of connections
* This can optionally ignore rewriting the outputs of cells or processes * This by default rewrites drivers of wires with public names but can also optionally rewrite drivers of wires with private names * A -remove flag allows cleaning up the design by replacing barriers with connections
* This uses optbarriers and flatten -barriers to insert barriers on public wires in the design, limiting the optimization that can affect them
* this is needed to handle $barrier cells which otherwise are similar to $buf but do not have a sign parameter
* this was added earlier in the PR with the intention of making equiv_opt work, but it means barriers are lost through passes like aigmap and can potentially be used in optimization passes so it is better to remove it
3bcca55 to
24a9cbe
Compare
|
@georgerennie since you've kept working on this PR on and off, I'm wondering if there are any parts left you consider still unfinished or that you'd like to discuss before any of this getting merged? (Apart from resolving the merge conflicts, of course.) I also still need to perform full review, but just from what we've discussed before and going by the commit messages, I'm wondering if it makes sense to split this PR into multiple parts so we can merge the core functionality, a minimal pass to add and remove barriers and the changes required for passes to respect the barriers first and merge more fine grained user control over barriers, backend support and attributes to represent weaker barriers in one or many follow up PRs. |
|
I think this is all ready to be merged in its current state (modulo the merge conflicts). I'm happy to break it up into a few PRs if that makes it easiest to review. Will close it and open PRs for the constituent parts (although I guess these will be stacked at least on the kernel changes but I can rebase once those land). |
|
I don't mind closing this PR as soon as the constituent parts have their own PRs, but until then I'd prefer to keep this open so it doesn't disappear from my |
This adds the
$barriercell type which represents an optimization barrier that Yosys should not optimize through. Changes include:$barrieras just a buffer. I also added$bufsupport while I was at it where it didn't exist.opt_mergeto ignore$barrier-barrierstoflattenthat will insert barriers to drive all public wires driven by the flatteningoptbarrierscommand to insert/remove barriers everywhere that public wires are driven. This has the following options:-nocells- don't add barriers for the outputs of cells-noprocs- don't add barriers for the outputs of processes-private- add barriers for private wires as well as public-remove- convert selected barriers back to connections-barriersflag toprepthat callsoptbarriersat the start and passes-barriersto flatten. This should be a drop in replacement for a regularprepcall that has the benefit of maintaining all the structure among public wires.This still needs tests and probably some discussion about what the flags/defaults should be as currently this is very conservative on the optimizations allowed. I wasn't sure exactly what optimizations people would want to opt into but attributes can be added to
$barrierto do so.